-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix usage of proxy.py in test_proxy_functional #7773
Fix usage of proxy.py in test_proxy_functional #7773
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7773 +/- ##
==========================================
+ Coverage 96.98% 97.40% +0.41%
==========================================
Files 107 107
Lines 32213 32207 -6
Branches 3744 3743 -1
==========================================
+ Hits 31243 31371 +128
+ Misses 756 632 -124
+ Partials 214 204 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Huh, I thought it was meant to be supported on all platforms. @webknjaz Happy to just have this test skipped? |
I think its only supported on all platforms after python/cpython#31275 |
That's a PR that appears to have been merged into 3.11+? |
Can it be that the warning mentioned @ #6044 (comment) is a result of a flawed logic there? I think I saw a problem in related code, but never got to fixing it.. Or is this about some other problem? |
I think thats a different issue. AFAICT the proxy isn't ssl so its not tls in tls #6044 (comment) The check might need to be Line 1248 in 30b7a4e
|
The way I read this is the test shouldn't run if that flag is set because tls in tls is supported on the version of python the test is running on, but before 3.11 monkey patching was needed to enable it |
Right, I hadn't really looked at the test. Doesn't that mean the test should just be skipped on Python 3.11+? And if so, why does it pass on Ubuntu with 3.11+? I'm a bit confused why the behaviour is different on different platforms. |
I think that's the case.
I would have expected it to be failing on Ubuntu 3.11. Maybe it's OpenSSL version dependent as well. I think I'm going to have to build a Ubuntu setup to be sure |
Marking as draft until I can put together a Ubuntu env for local testing |
Unfortunately I'm getting different behavior on the ubuntu docker container. It might be openssl version dependent or the fact I'm running in a docker host. Instead I adjusted the other failing test to run if we think tls in tls is supported so we have:
|
If |
No problem. @webknjaz had a good tool for that, a github action that allows you to login to the server or something. I don't remember what it was called, but a quick search suggests it might be one of these 2: https://github.com/marketplace/actions/a-debugger-for-actions https://github.com/marketplace/actions/debug-via-ssh |
saved |
679ac97
to
d0d5790
Compare
…_tls_in_tls_macos' into fix_test_https_proxy_unsupported_tls_in_tls_macos
Looks like this is good to go The only one still failing reliably (and not related to this PR) is tests/test_web_server.py::test_unsupported_upgrade[pyloop] - TimeoutError on windows and mac only |
Great, one step closer. |
Backport to 3.9: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply 4d9fc63 on top of patchback/backports/3.9/4d9fc636dbad45678330f17b7d82b75cf91247bf/pr-7773 Backporting merged PR #7773 into master
🤖 @patchback |
Backport to 3.10: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply 4d9fc63 on top of patchback/backports/3.10/4d9fc636dbad45678330f17b7d82b75cf91247bf/pr-7773 Backporting merged PR #7773 into master
🤖 @patchback |
I'll get the patch backs done after breakfast and coffee. Up early due to jet lag |
(cherry picked from commit 4d9fc63)
(cherry picked from commit 4d9fc63)
@Dreamsorcerer it's this one https://github.com/marketplace/actions/debugging-with-tmate. I recommend using the options to restrict the user who can connect with it. |
@bdraco thanks for figuring this out! I still remember how exhausting it was to integrate this in the first place… |
Cheers! This one was a bit of a rabbit hole. I can only imagine how it was to integrate in the first place 😉 |
@bdraco so I just realized that I accidentally downgraded the proxy.py version at the beginning of August via these PRs:
We should probably recover allowing pre-releases there to get the latest fixes early, as that project doesn't release often, despite my extensive contributions to their automation. |
I haven't used pip-tools before so I'm not 100% sure if this is the right way to go about it, but #7927 gives it a shot |
@bdraco that's good enough. I don't think we have the exact invocation documented + we don't normally run it manually — dependabot uses pip-tools to upgrade pairs of |
This test was running even if tls in tls was supported without the need to monkey patch because the failures were actually problems with usage of
proxy.py
.The test would fail because of problems with usage of
proxy.py
, and not because tls in tls didn't work. This one turned out to have a lot of side trips to discover why things failed because proxy.py doesn't expect to run inside pytest, and is not easy to get error reporting out of when run this way.DeprecationWarning: ssl.OP_NO_SSL*/ssl.OP_NO_TLS* options are deprecated
--threadless
does not work with proxy.py [Threadless] OSError: [WinError 10038] an operation was attempted on something that is not a socket abhinavsingh/proxy.py#492